-
-
Notifications
You must be signed in to change notification settings - Fork 717
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RFC] Decompose server class #8468
base: main
Are you sure you want to change the base?
Conversation
b8b217b
to
395fcc2
Compare
Some general comments without diving too deep into the actual changes: +1 on refactoring more of our codebase away from inheritance to composition where possible/useful. IMO, this also makes testing easier. |
Unit Test ResultsSee test report for an extended history of previous test failures. This is useful for diagnosing flaky tests. 29 files ± 0 29 suites ±0 14h 35m 48s ⏱️ + 3h 18m 41s For more details on these failures and errors, see this check. Results for commit f8ccddf. ± Comparison against base commit 8564dc7. This pull request removes 10 and adds 10 tests. Note that renamed tests count towards both.
This pull request skips 2 tests.
♻️ This comment has been updated with latest results. |
395fcc2
to
fcaee9d
Compare
I was thinking a little more about how #8430 could be used and ended up wanting to use this for the client as well.
(This is also loosely motivated by dask/dask-expr#765 where I suggested to move the key generation / tokenization from client to scheduler where the ordered sendrcv of #8430 would be nice)
The client unfortunately re-implements the RPC stream handler of the server base class making it hard to reuse the ordered sendrcv without a lot of code duplication.
This PR suggests a refactoring where I propose to move from inheritance to composition regarding the
Server
networking and RPC logic. The currentServer
base class is mixing a lot of application logic with networking logic which in the past frequently caused issues during teardown so separation of those concerns is long overdue.The
Server
as proposed in this PR should likely rather be calledRPCServer
orRPCSomethingElse
since it doesn't actually implement a traditional server<->client model but I'm trying to not get hung up on naming at this point.With this decomposition I was then able to use the slimmed down
Server
in the client as well which opens up the reuse of smth like #8430This refactoring is not really done but if we go down this road, the universally used
ConnectionPool
andstream_comms
would basically blend into this single object which would support three ways to communicateState of this PR: Mostly working but still a couple of failing tests. From what I saw nothing insurmountable.
There are two commits
I think I'm mostly interested in feedback about the first one since that may have merit without the introduction of new features